feat(graphql-server): attribute a request to the API's entity, as a whole pair - #1775
feat(graphql-server): attribute a request to the API's entity, as a whole pair#1775pyramation wants to merge 1 commit into
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
Review complete. 🟡 1 medium 💬 Inline comments (1)
🧹 Nitpicks (1) — 🟢 1 low
The change introduces API_ENTITY_TYPE, parsed into api.entityType in graphql/env/src/env.ts, and wires it through ApiOptions into the PostGraphile preset and finally into the request-context JWT claim builder in graphql/server/src/middleware/graphile.ts, where entity_id/entity_type claims are set alongside database_id only when both apiEntityType and req.databaseId are present. A new test file covers the attribution logic. The implementation is functionally correct for the happy path and consistently guarded.
Reviewed commit: 5d8c6ae |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds an API_ENTITY_TYPE env var that threads an entity type through the GraphQL env options, preset, and request-context JWT claim attribution, plus a new test for the attribution logic.
Key findings
- 🟡 Test duplicates logic instead of exercising middleware — graphile-entity-attribution.test.ts:4
| const buildContext = ( | ||
| req: Partial<Request>, | ||
| entityType?: string | ||
| ): Record<string, string> => { | ||
| const context: Record<string, string> = {}; | ||
|
|
||
| if (entityType && req.databaseId) { | ||
| context['jwt.claims.entity_id'] = req.databaseId; | ||
| context['jwt.claims.entity_type'] = entityType; | ||
| } | ||
|
|
||
| if (req.databaseId) { | ||
| context['jwt.claims.database_id'] = req.databaseId; | ||
| } | ||
|
|
||
| return context; | ||
| }; |
There was a problem hiding this comment.
🟡 bug · medium
Test duplicates logic instead of exercising middleware
The new test graphile-entity-attribution.test.ts re-implements the claim-building logic in a local buildContext helper (graphile-entity-attribution.test.ts:4-20) that mirrors graphile.ts:218-221 and asserts only against that copy, never importing or invoking the real buildPreset context builder. Because the test and production code are separate copies, a regression in the middleware's attribution logic (wrong predicate, dropped claim, mis-ordered override) would leave these tests green, so the suite provides no protection for the behavior it claims to verify.
📋 Prompt for AI Agents
In graphql/server/src/middleware/graphile.ts, extract the context-building block (currently the if (req) { ... } that sets jwt.claims.entity_id/entity_type/database_id/api_id etc., around lines 217-243) into an exported pure helper such as export function buildJwtClaimContext(req, apiEntityType): Record<string,string>, and call it from the grafast context callback. Then rewrite graphql/server/src/middleware/__tests__/graphile-entity-attribution.test.ts to import and assert against that exported helper (ideally plus an integration test that drives a request through the middleware) instead of re-implementing the same logic in a local buildContext, so the test exercises the real production code path and catches regressions in the attribution logic.
1 participant
Summary
A GraphQL request already tells the database who is acting (
jwt.claims.user_id) and which database it landed on, but never which entity the work belongs to. Downstream (constructive-db) now refuses to create work — jobs, invocations — that has neither an actor nor an entity, so any request that enqueues work through a surface without an authenticated user fails withATTRIBUTION_REQUIRED, and even authenticated work is unattributable for billing. The server is the only place that can supply it: it owns route resolution,req.databaseId, and the request transaction.So
buildPresetnow stamps the entity pair into the same sharedcontextas the other provenance claims, before the authenticated/anonymous split, so both branches carry it:Two constraints worth stating, because both are load-bearing:
entity_idwithout a type has to guess the type, and a defaulted'platform'is a lie for every other surface. Hence no fallback entity type and no set-one-without-the-other path.process.env. It arrives asapi.entityTypethroughApiOptions/getGraphQLEnvVars(API_ENTITY_TYPE), perAGENTS.md's rule that config is read through the env options system, not scraped at the point of use. Unset means the server stamps nothing and behaves exactly as before — this is opt-in per deployment, since only the deployment knows whether its surface isplatform,database, or something else.Tests cover the three cases that matter: both present → complete pair; entity type unset → no entity claims;
databaseIdmissing → no entity claims.Link to Devin session: https://app.devin.ai/sessions/47477486a4fd45e684bd663b7007a629
Requested by: @pyramation